-
-
Notifications
You must be signed in to change notification settings - Fork 732
remove redundant "else" test in temperature example #451
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fix arduino#450. There is also a spelling problem with "precede".
{ | ||
//Warning! User attention required | ||
// Warning! Thorttle down, run the fans, and notify the user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You introduced a typo here (should be spelled "Throttle"). I'm actually not convinced changing this comment provides any benefit, though I do very much agree with the change to a consistent comment style.
I would recommend not removing the redundant part of the expression. The code without it may be counter-intuitive for beginners. Besides, if you add more cases (temperature ranges) to the example, you probably would have to put it back. I instead recommend just reorganizing the code as follows:
|
I am a beginner and raised the related issue. My point in reading the example was to understand what the difference between "if" and "else if" is. And I still think, the current example does not show the benefit of not having to use redundant checks, if the "above" statements were already checked. In the example of @robsoncouto you could also just use "if" statements IMHO. What about using an example something like this: |
Hey @per1234 , could you please fix this one as you think it is adequate? |
My concern with the current code and your suggested code even more so is not about efficiency, but ease of code maintenance. Let's imagine the programmer decides they want to reduce the danger temperature threshold to 65. With your suggested code, they need to remember to change the value in two different places. If they forget to change one of the two values (a very easy thing to do when you have a lot of code in the clause): if (temperature < 60)
{
// Safe! Continue usual tasks...
}
else if (temperature >= 60 && temperature < 70)
{
// Warning! Throttle down, run the fans, and notify the user
}
else if (temperature >= 65)
{
// Danger! Shut down the system
} then the intended change in behavior was not accomplished. Of course if you used variables instead it wouldn't be an issue, but adding variables to the example code introduces extra complexity, which is contrary to your intent of making it easier for a beginner to understand. My suggestion is to use best practices in the code, but add comments to make it clear how the code works. Something like this: if (temperature >= 70)
{
// Danger! Shut down the system
}
else if (temperature >= 60) // 60 <= temperature < 70
{
// Warning! User attention required
}
else // temperature < 60
{
// Safe! Continue usual tasks...
} |
Updated and merged. |
Fix #450. There is also a spelling problem with "precede".